feat: allow overriding service name in tracer, meter, and logger config#2883
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds configurable OpenTelemetry service names for logger, tracer, and meter (default "permify"), exposed via YAML config, CLI flags ( Changes
Sequence Diagram(s)sequenceDiagram
participant User as CLI / Config
participant Viper as Viper (flags/env)
participant Config as internal/config
participant Serve as pkg/cmd/serve
participant Telemetry as pkg/telemetry
participant OTLP as OTLP Exporter
User->>Viper: provide flags, env or YAML
Viper->>Config: populate config (ServiceName fields)
Config->>Serve: pass cfg with Logger/Tracer/Meter.ServiceName
Serve->>Telemetry: initialize Logger/Tracer/Meter (pass serviceName)
Telemetry->>OTLP: create resources with semconv.service.name = serviceName
OTLP-->>Telemetry: exporter initialized
Telemetry-->>Serve: initialization complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
example.config.yaml (1)
24-27:⚠️ Potential issue | 🟡 MinorMissing
service_namein logger section.The
loggersection is missing theservice_namefield, but it's been added to bothtracer(line 48) andmeter(line 56). For consistency and to match the documentation and config struct changes, addservice_name: permifyhere as well.Proposed fix
# The logger section sets the logging level for the service. logger: level: info file: "" # Log file path (empty for stdout) + service_name: permify🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@example.config.yaml` around lines 24 - 27, The logger configuration is missing the service_name field; add service_name: permify under the logger section so it matches the tracer and meter sections and the updated config struct—specifically update the logger block (logger:, level:, file:) to include the service_name key with value "permify" to ensure consistency with tracer and meter entries.
🧹 Nitpick comments (1)
pkg/telemetry/log.go (1)
90-95: Add an empty-value fallback forserviceName.At Line 94,
service.nameis set directly from input. If a blank value slips in via config/env/flag, telemetry may emit an empty service name and break service-level dashboards. Consider normalizing to the default before building the resource.Proposed defensive patch
import ( "context" "errors" "fmt" "log/slog" "os" "runtime" + "strings" @@ func newResource(serviceName string) *resource.Resource { + if strings.TrimSpace(serviceName) == "" { + serviceName = internal.Identifier + } hostName, _ := os.Hostname() return resource.NewWithAttributes( semconv.SchemaURL, semconv.ServiceNameKey.String(serviceName),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/telemetry/log.go` around lines 90 - 95, The newResource function sets semconv.ServiceNameKey directly from the serviceName parameter which can be blank; add a defensive fallback that normalizes serviceName to a non-empty default before building the resource (e.g., if serviceName == "" then assign a constant like defaultServiceName or "unknown_service"), then use that normalized value when calling resource.NewWithAttributes in newResource.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@example.config.yaml`:
- Around line 24-27: The logger configuration is missing the service_name field;
add service_name: permify under the logger section so it matches the tracer and
meter sections and the updated config struct—specifically update the logger
block (logger:, level:, file:) to include the service_name key with value
"permify" to ensure consistency with tracer and meter entries.
---
Nitpick comments:
In `@pkg/telemetry/log.go`:
- Around line 90-95: The newResource function sets semconv.ServiceNameKey
directly from the serviceName parameter which can be blank; add a defensive
fallback that normalizes serviceName to a non-empty default before building the
resource (e.g., if serviceName == "" then assign a constant like
defaultServiceName or "unknown_service"), then use that normalized value when
calling resource.NewWithAttributes in newResource.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7ba65165-5ffb-465d-af6a-9fe9b736127b
📒 Files selected for processing (9)
docs/setting-up/configuration.mdxexample.config.yamlinternal/config/config.gointernal/config/config_test.gopkg/cmd/flags/serve.gopkg/cmd/serve.gopkg/telemetry/log.gopkg/telemetry/meter.gopkg/telemetry/tracer.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/telemetry/log.go (1)
92-94: NormalizeserviceNamebefore assigningservice.name.Current logic only checks
TrimSpacefor emptiness. A value like" my-service "will be emitted with spaces and appear as a distinct service in telemetry backends. Trim once, then apply fallback.Suggested patch
func newResource(serviceName string) *resource.Resource { - if strings.TrimSpace(serviceName) == "" { + serviceName = strings.TrimSpace(serviceName) + if serviceName == "" { serviceName = "permify" } hostName, _ := os.Hostname() return resource.NewWithAttributes( semconv.SchemaURL, semconv.ServiceNameKey.String(serviceName),Also applies to: 98-98
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/telemetry/log.go` around lines 92 - 94, Trim serviceName once and use the trimmed value for both the emptiness check and the subsequent assignment to service.name: replace the current strings.TrimSpace check with a single trimmed := strings.TrimSpace(serviceName) then if trimmed == "" set serviceName = "permify" else set serviceName = trimmed; do the same change for the other occurrence that assigns service.name so no leading/trailing spaces are emitted to telemetry.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/telemetry/log.go`:
- Around line 92-94: Trim serviceName once and use the trimmed value for both
the emptiness check and the subsequent assignment to service.name: replace the
current strings.TrimSpace check with a single trimmed :=
strings.TrimSpace(serviceName) then if trimmed == "" set serviceName = "permify"
else set serviceName = trimmed; do the same change for the other occurrence that
assigns service.name so no leading/trailing spaces are emitted to telemetry.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 71720bb0-422c-4009-89ba-cf070c4bec0e
📒 Files selected for processing (3)
pkg/telemetry/log.gopkg/telemetry/meter.gopkg/telemetry/tracer.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/telemetry/tracer.go
- pkg/telemetry/meter.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2883 +/- ##
==========================================
+ Coverage 82.58% 82.58% +0.01%
==========================================
Files 74 74
Lines 8297 8300 +3
==========================================
+ Hits 6851 6854 +3
Misses 910 910
Partials 536 536 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
omer-topal
left a comment
omer-topal
left a comment
There was a problem hiding this comment.
Could you also add the new logger.service_name, tracer.service_name, and meter.service_name fields to permify config output as well?
Oups sorry about that |
2 participants
Summary
Closes #2877
Adds a
service_nameconfiguration option undertracer,meter, andloggersections, allowing users to override the service name reported by each exporter. Defaults to"permify"for backward compatibility.Configuration
CLI flags:
--tracer-service-name,--meter-service-name,--log-service-nameEnv vars:
PERMIFY_TRACER_SERVICE_NAME,PERMIFY_METER_SERVICE_NAME,PERMIFY_LOG_SERVICE_NAMETest plan
Automated
go build ./...— clean buildgo test ./internal/config/...— 9/9 pass (2 new tests for service name default + override)Manual testing
Tested locally using
grafana/otel-lgtmall-in-one (Grafana + Loki + Tempo + Prometheus + OTel Collector):Then open http://localhost:3000 and verify:
{service_name="my-custom-service"}— logs appear with correct service name{job="my-custom-service"}— runtime/host metrics appearmy-custom-service— traces appearScreenshot — Logs in Grafana showing
my-custom-service:Screenshot — Metrics showing
my-custom-service:Screenshot — Traces in Grafana / Jaeger showing
my-custom-service:Summary by CodeRabbit
New Features
Documentation
Tests
Chores